-
Notifications
You must be signed in to change notification settings - Fork 390
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Count repos across federation members in quota #1461
base: main
Are you sure you want to change the base?
Conversation
Make binderspawner_mixin.py canonical, instead of python-in-yaml easier to edit with editors, linters, etc. removes use of redundant pyyaml
for easier access than env
When you say "costly" do you mean in terms of infrastructure costs? My intuition is that in the long term, our people costs are still much higher than our cloud costs, and so I am generally +1 on taking "simpler approaches that are a bit costlier in cloud". This seems like a good iterative step forward on what we currently have to me. |
@@ -612,6 +613,41 @@ def _default_build_token_secret(self): | |||
""" | |||
) | |||
|
|||
federation_id = Unicode( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it feels a bit weird that we are now encoding the notion of a "federation" in BinderHub itself - do we imagine any other usecase than mybinder.org where a user would want this? Or are we OK with designing BinderHub features that are just for mybinder.org?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main difference between the two - this PR requires BinderHubs to 'pull' quotas and know about its peers, whereas #1460 uses a 'push' model, which works if we tell all the instances to push their quota consumption to the same place.
I can also adjust #1460 to change less - keep the concurrent-session quota counting instead of also switching to rate limiting. But then frankly, we are starting to reimplement a KV store, and should probably deploy etcd/memcache/redis to handle that.
I mean performance-wise (making things slower, not costing more money). This approach moves larger amounts of information around and possibly making more k8s API calls (I don't think so, since it's piggy-backing on the same calls used by the health check already called every 10s by federation-redirect). On the other hand, #1460 results in additional HTTP requests for every build, so that might slow things down itself. |
Without understanding the technical details that much, my vote is for whatever is conceptually simplest, and has the least amount of maintenance burden over time. It seems like that is this PR? |
This is an alternative to #1460, also for jupyterhub/mybinder.org-deploy#2143
Unlike #1460, it keeps all the definitions of quotas and such, except:
This is quite a bit simpler thank #1460, but potentially more costly (I'm not sure)
Additionally, I placed the binder repo_url in pod annotations, for easier lookup.
includes #1459